[WIP] Issue#98: refactor test suite #294
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
tests/test_multidict_.py
Outdated
| class TestContents: | ||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def _autoassign_md_to_d(self, md_w_multivalue_per_key): |
There was a problem hiding this comment.
I like the parametrization approach but this particular fixture looks like a hack.
Instead of self.d we can always use a fixture for multidict instance explicitly.
This allows getting rid of self.d entirely.
self.d is necessary for unittest based approach but redundant for pytest
There was a problem hiding this comment.
Also, classes are normally not necessary. If you need namespacing, flat is better than nested: just use separate test modules :)
There was a problem hiding this comment.
Agree but I don't insist.
Can live with classes if they are already exist.
There was a problem hiding this comment.
Yeah, but classes often implicitly encourage ppl to have state. Which might lead to tests depending on each other. Why rely on human reviewers to catch those when simple rules are more effective in terms of discouraging this? :)
There was a problem hiding this comment.
Is getting rid of classes the suggestion or requirement?
There was a problem hiding this comment.
Suggestion. A requirement would need actual linting implementation.
There was a problem hiding this comment.
(But still, it's also: simple is better than complex)
There was a problem hiding this comment.
@webknjaz regarding classes -> modules part:
I mostly agree but it is hard to split something in modules when you don't actually know all parts.
Like here I am still in progress figuring out the layout and all needed fixtures.
When it is settled - I will probably come back to that thought.
tests/test_multidict_.py
Outdated
| assert list(d.items()) == [] | ||
|
|
||
| assert cls() != list() | ||
| with pytest.raises(TypeError, match='\(2 given\)'): |
There was a problem hiding this comment.
please use raw-string literals instead of escaping everything
|
hey @asvetlov , just a friendly ping. |
webknjaz
added
the
bot:chronographer:skip
|
If this is still relevant after #915, please recreate the PR with those fixtures. |
Disclaimer: work in progress. I need initial review to proceed.
Main points:
[MultiDict, CIMultiDict, MultiDictProxy, CIMultiDictProxy]) are pre-created and used for tests which concern not instantiation of those classes themselves(unfortunately, pytest so far [doesn't support fixtures in pytest.mark.parametrize] )(Using fixtures in pytest.mark.parametrize pytest-dev/pytest#349)
useautouse=Trueclass level hack (since we can't use__init__) to be able to useself.dinstead of long fixture names.Since my goal is restructuring - I didn't touch asserts themselves. I think that such comments can be addressed after the fact.